feat: gradually add metrics capabilities to Instrumentation::Base #1324
Conversation
zvkemp
force-pushed
the
base-metrics
branch
2 times, most recently
from
7def994 to
f060a57
Compare
zvkemp
force-pushed
the
base-metrics
branch
3 times, most recently
from
bccd9f5 to
5f393f8
Compare
zvkemp
force-pushed
the
base-metrics
branch
3 times, most recently
from
ef747e7 to
8a538f8
Compare
zvkemp
changed the title
zvkemp
requested review from
fbogsany,
mwear,
robertlaurin,
dazuma,
ericmustin,
arielvalentin,
ahayworth,
plantfansam,
robbkidd,
simi,
kaylareopelle,
xuan-cao-swi and
a team
as code owners
|
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
|
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
|
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
kaylareopelle
added
keep
| @@ -163,8 +164,10 @@ def option(name, default:, validate:) | |||
| end | |||
|
|
|||
| def instance | |||
| @instance ||= new(instrumentation_name, instrumentation_version, install_blk, | |||
| present_blk, compatible_blk, options) | |||
| @instance || SINGLETON_MUTEX.synchronize do | |||
There was a problem hiding this comment.
Can you explain the rational for why this mutex lock is necessary?
| module Instrumentation | ||
| # Extensions to Instrumentation::Base that handle metrics instruments. | ||
| # The goal here is to allow metrics to be added gradually to instrumentation libraries, | ||
| # without requiring that the metrics-sdk or metrics-api gems are present in the bundle |
There was a problem hiding this comment.
without requiring that the metrics-sdk or metrics-api gems are present in the bundle
Instrumentation base has a dependency on the opentelemetry-api gem, if we want to start adding metric support in the instrumentation libraries the way forward is to get the metrics api stable and merged in the api proper.
| !disabled_by_env_var? | ||
| end | ||
|
|
||
| def disabled_by_env_var? |
There was a problem hiding this comment.
I don't actually see where this is being used.
Proposed implementation for adding optional metrics capabilities to existing instrumentation libraries.
See #1377, #1314, or zvkemp#1 (example of observables) for example implementations. (<- note that these PRs are based on top of this branch, so you may see some duplication between here and there)
Proposed api:
:metrics.TODO:
Kernel.requireand detect when Metrics is loaded (probably too perilous to really consider, as any instrumentations with conditional metrics will need to be re-evaluated at this time), or eagerly attempt to require the metrics-api gem when Instrumentation::Base is loaded.